HADOOP-18740. S3A prefetch cache blocks should be accessed by RW locks#5675
Merged
steveloughran merged 4 commits intoapache:trunkfrom Jun 7, 2023
Merged
HADOOP-18740. S3A prefetch cache blocks should be accessed by RW locks#5675steveloughran merged 4 commits intoapache:trunkfrom
steveloughran merged 4 commits intoapache:trunkfrom
Conversation
Contributor
Author
|
All failures related to HADOOP-18744 |
|
💔 -1 overall
This message was automatically generated. |
|
I want to implement a new cache replacement algorithm in hdfs but I don't know which part of module to use and modify. Can any one please help me find the modules and functions which need to override to add my own caching replacement algorithm in hdfs |
Contributor
|
@MayankSinghParmar get on the hadoop hdfs mailing list and discuss there. @virajjasani can you rebase and retest? |
steveloughran
requested changes
May 31, 2023
Contributor
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
core changes look good, just a nit about using this. where it isn't needed.
I'm worried about concurrency of close(), where
- we should make the
closedflag atomic boolean; only execute close on the first call and have all the other ops fail if closed - be confident that locking in close() is good. After the disaster with the abfs prefetcher, we need to look carefully here
| */ | ||
| void takeLock(LockType lockType) { | ||
| if (LockType.READ == lockType) { | ||
| this.lock.readLock().lock(); |
| */ | ||
| void releaseLock(LockType lockType) { | ||
| if (LockType.READ == lockType) { | ||
| this.lock.readLock().unlock(); |
| int numFilesDeleted = 0; | ||
|
|
||
| for (Entry entry : blocks.values()) { | ||
| entry.takeLock(Entry.LockType.WRITE); |
Contributor
There was a problem hiding this comment.
should we be acquiring locks in close()?
good: no race condition in close
bad) the usual
also, L303: should closed be atomic?
Contributor
Author
There was a problem hiding this comment.
also, L303: should closed be atomic?
+1 to this suggestion, let me create a separate patch with HADOOP-18756 to better track it.
good: no race condition in close
bad) the usual
sounds reasonable, let me try setting timeout
Contributor
Author
|
Test results are clean |
|
💔 -1 overall
This message was automatically generated. |
steveloughran
requested changes
Jun 1, 2023
|
|
||
| for (Entry entry : blocks.values()) { | ||
| entry.takeLock(Entry.LockType.WRITE); | ||
| boolean lockAcquired = entry.takeLock(Entry.LockType.WRITE, 5, TimeUnit.SECONDS); |
Contributor
There was a problem hiding this comment.
pull the number into a constant. Know that I automatically -1 all inline constants in production code and save time all round.
Contributor
Author
There was a problem hiding this comment.
my bad, let me fix this real quick
| * @param unit the time unit of the timeout argument. | ||
| * @return true if the lock of the given lock type was acquired. | ||
| */ | ||
| boolean takeLock(LockType lockType, long timeout, TimeUnit unit) { |
Contributor
There was a problem hiding this comment.
should this and the others be private? you don't want other classes playing with your lock code...
Contributor
Author
There was a problem hiding this comment.
sounds good, done
|
💔 -1 overall
This message was automatically generated. |
steveloughran
approved these changes
Jun 7, 2023
Contributor
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
+1. concurrency stuff always worries me, but this looks good
asfgit
pushed a commit
that referenced
this pull request
Jun 8, 2023
#5675) Contributed by Viraj Jasani
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Jira: HADOOP-18740